Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide an option to use Emacs advice system #63

Merged
merged 7 commits into from
Feb 26, 2023

Conversation

haji-ali
Copy link
Contributor

Advising is only done when the name of the function is the same as the original one and if el-patch-use-advice contains the relevant type (like defun). In this case, a function *@el-patch-advice is defined and the original function is advised.

Things to consider:

  • How this plays with variants (I don't use them, so I am out of the loop).
  • If we want to include a field in el-patch-deftype-alist that distinguishes advisable types from the rest, to avoid having the wrong type in el-patch-use-advice.
  • Currently, I don't change anything in el-patch--patches to distinguish between advised vs patched functions.
  • There is currently no way to advice some functions but not others (related to the previous point). Perhaps this is desirable?

Once we discuss these points, I will finalize this pull request.

@haji-ali
Copy link
Contributor Author

A bigger issue that I am not sure we can resolve is this example:

(require 'el-patch)

(setq el-patch-use-advice '(defun))

(defun my/test (st)
  (message (format "Hello %s" st)))

(el-patch-defun my/test (st
                         (el-patch-add st2))
  (message (format (el-patch-swap "Hello %s"
                                  "Testing %s %s")
                   st (el-patch-add st2))))

(my/test "big" "world")

This seems to work as expected. However, byte-compiling produces a warning on the expected number of arguments.
Perhaps we should also require the exact same arguments when advising? I don't see an easy way to allow more complicated argument specification.

@raxod502
Copy link
Member

I think there's no real way to resolve that issue. You'd encounter the same problem if you used an advice normally to change the number of arguments taken by a function.

There might be a workaround with some more sophisticated pointers given to the byte-compiler, but I don't know what it would be offhand, and I don't think the behavior as implemented is problematic.

How this plays with variants (I don't use them, so I am out of the loop).

I think what we want is to use the current value of the variable el-patch-variant in the name of the advice function that is defined when executing el-patch-defun. If we do that, then variants should work as expected.

If we want to include a field in el-patch-deftype-alist that distinguishes advisable types from the rest, to avoid having the wrong type in el-patch-use-advice.

That seems prudent. On the other hand, perhaps we could synthesize this information automatically by inspecting the return value of the :classify function and saying that a form can only be patched using advice if it defines a single symbol of type function?

Currently, I don't change anything in el-patch--patches to distinguish between advised vs patched functions.

Can we treat advising versus patching in the same way as variants? That is, there is another layer of sub-hashtable in el-patch--patches, and when invoking interacting commands that act on a patch, if there is both a patch and an advice for a symbol, then you can choose, but otherwise it defaults to the single one that's available?

There is currently no way to advice some functions but not others (related to the previous point). Perhaps this is desirable?

What if we make the interface a simple boolean user option to toggle the mode? You can then set it globally (to only apply to form types that are advisable), or bind it dynamically around a single patch. That offers the maximum flexibility, while solving for the simple case transparently, using the same paradigm as is already used in the variant system.

@haji-ali
Copy link
Contributor Author

haji-ali commented Feb 5, 2023

I think what we want is to use the current value of the variable el-patch-variant in the name of the advice function that is defined when executing el-patch-defun. If we do that, then variants should work as expected.

Hmm... if we want to allow mixing the two patching systems, then on every call el-patch--definition, we need to evaluate the original definition and then define and add the advice. Also, we need to remove the previous advice, if any.

Can we treat advising versus patching in the same way as variants? That is, there is another layer of sub-hashtable in el-patch--patches, and when invoking interacting commands that act on a patch, if there is both a patch and an advice for a symbol, then you can choose, but otherwise it defaults to the single one that's available?

Sounds reasonable. How about we use (variant . advice) where advice is either nil or t, as the key for the hashtable instead of creating a new one?

We would also need a way to figure out the last applied patch (advice or not)

That seems prudent. On the other hand, perhaps we could synthesize this information automatically by inspecting the return value of the :classify function and saying that a form can only be patched using advice if it defines a single symbol of type function?
...
What if we make the interface a simple boolean user option to toggle the mode? You can then set it globally (to only apply to form types that are advisable), or bind it dynamically around a single patch. That offers the maximum flexibility, while solving for the simple case transparently, using the same paradigm as is already used in the variant system.

Yep, sounds good. Implemented now.

@haji-ali
Copy link
Contributor Author

haji-ali commented Feb 7, 2023

OK, I implemented it as I suggested (using (advice . variant) as keys). I think my concerns regarding un-patching are unwarranted. I am adding a few comments on different code lines. @raxod502, Let me know what you think.

I am dogfooding this change and so far it seems to be working well.

el-patch.el Outdated
@@ -222,6 +226,13 @@ This function lives halfway between `copy-sequence' and
(cons (car tree) (el-patch--copy-semitree (cdr tree)))
tree))

(defun el-patch--advice-name (name variant)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be confusing since everywhere else, variant is already (advice . variant-name) except here where it's just variant-name.

Comment on lines +635 to +636
(advice-remove name
(el-patch--advice-name name (cdr variant)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that an advice takes precedence regardless, even if a patch was applied after it. It must be manually removed or un-patched.

Comment on lines +559 to +560
;; Patches must have the same name and
;; same number of arguments
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue is that there's no way to tell the use that this patch was not made into an advice despite having non-nil el-patch-use-advice. I thought about showing a warning, but we would need a way to silence those warnings in init-files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay to have it be silent at least for now. We can add a way to communicate the information later, if people report confusion.

Comment on lines -936 to +995
(let ((el-patch-variant variant))
(unless (el-patch-validate name type 'nomsg)
(setq warning-count (1+ warning-count)))))))))
(unless (el-patch-validate name type 'nomsg nil
variant)
(setq warning-count (1+ warning-count))))))))
Copy link
Contributor Author

@haji-ali haji-ali Feb 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be mistaken, but I think this was a bug. Setting el-patch-variant does not change the behavior of el-patch-validate. Instead the variant should be passed as an argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Thanks for fixing that.

(puthash ',type
(gethash
',type table
(make-hash-table :test #'equal))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to change the test function for the variant hashtable. Nevertheless, we should check that el-patch-variant is not a cons or a string.

el-patch.el Outdated Show resolved Hide resolved
Copy link
Member

@raxod502 raxod502 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ I'm sorry this took so long. Had a lot of problems people were reporting with GNU ELPA Mirror that took some weeks to resolve. The approach looks good. There might be problems in some edge cases but they can be addressed in the future if discovered.

el-patch.el Outdated Show resolved Hide resolved
el-patch.el Outdated
@@ -151,6 +151,10 @@ loaded. You can toggle the `use-package' integration later using
"Non-nil means to validate patches when byte-compiling."
:type 'boolean)

(defcustom el-patch-use-advice nil
"Non-nil causes el-patch to use Emacs' advice system for patching."
:type 'list)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a list then the docstring should probably mention what happens when you set it to that value (as opposed to just non-nil).

Comment on lines -936 to +995
(let ((el-patch-variant variant))
(unless (el-patch-validate name type 'nomsg)
(setq warning-count (1+ warning-count)))))))))
(unless (el-patch-validate name type 'nomsg nil
variant)
(setq warning-count (1+ warning-count))))))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Thanks for fixing that.

Comment on lines +559 to +560
;; Patches must have the same name and
;; same number of arguments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's okay to have it be silent at least for now. We can add a way to communicate the information later, if people report confusion.

@haji-ali haji-ali marked this pull request as ready for review February 26, 2023 18:40
@haji-ali haji-ali merged commit c4a1b43 into radian-software:main Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants